Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: integrate NodeTypesWithDB #10698

Merged
merged 39 commits into from
Sep 5, 2024
Merged

feat: integrate NodeTypesWithDB #10698

merged 39 commits into from
Sep 5, 2024

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Sep 4, 2024

following #10683 integrates NodeTypesWithDB for use in providers and to types depending on them.

This introcuces a lot of ChainSpec = ChainSpec bounds comming from concrete provider implementations. I've created a helper trait ProviderNodeTypes

pub trait ProviderNodeTypes: NodeTypesWithDB<ChainSpec = ChainSpec> {}

this trait can be removed/relaxed once we figure out requirements for abstracted chainspec

@joshieDo joshieDo added the A-sdk Related to reth's use as a library label Sep 4, 2024
@klkvr klkvr force-pushed the klkvr/integrate-node-types branch from 26ee4fd to acc020e Compare September 5, 2024 01:44
@@ -63,14 +64,14 @@ pub type PipelineWithResult<DB> = (Pipeline<DB>, Result<ControlFlow, PipelineErr
/// # Defaults
///
/// The [`DefaultStages`](crate::sets::DefaultStages) are used to fully sync reth.
pub struct Pipeline<DB: Database> {
pub struct Pipeline<N: NodeTypesWithDB> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not clear to me why we need the full node types here?

Copy link
Member Author

@klkvr klkvr Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically it's needed because pipeline currently depends on concrete ProviderFactory which needs chainspec AT

we can indeed avoid this by doing <DB, Provider: DatabaseProviderFactory<DB> + ...>, but given that we're going to make providers generic over primitive types this DB generic would at some point become NodeTypesWithDB or equivalent anyway

@klkvr klkvr force-pushed the klkvr/integrate-node-types branch from c5f3f83 to 12b5d45 Compare September 5, 2024 14:08
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice,

granted this introduces additional verbosity, but this is necessary if we want to integrate type generics on provider/db level

there are def some simplifications we can when it comes to the NodeWith trait, but imo it's nice to group db+types in on trait and the caller is responsible to instantiate this
the nice benefit of this is that we can get rid of a ton of DB generics, which is nice.
we also only can use the DB together with types so it makes sense to group those imo.

followup simplifications we can try are

  • stacked trait bounds for engine, this requires some nodebuilder changes which we def need to do
  • move away from ChainSpec as the default type

@mattsse mattsse marked this pull request as ready for review September 5, 2024 15:07
@klkvr klkvr changed the title [wip] feat: integrate NodeTypesWithDB feat: integrate NodeTypesWithDB Sep 5, 2024
@joshieDo joshieDo added this pull request to the merge queue Sep 5, 2024
Merged via the queue into main with commit 5ecc9d2 Sep 5, 2024
35 checks passed
@joshieDo joshieDo deleted the klkvr/integrate-node-types branch September 5, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sdk Related to reth's use as a library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants